Add process tags and container id to process discovery#5336
Add process tags and container id to process discovery#5336
Conversation
|
Thank you for updating Change log entry section 👏 Visited at: 2026-02-09 22:30:22 UTC |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 87ab619 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
… enabled per the system tests.
wantsui
added
the
AI Generated
p-datadog
left a comment
p-datadog
left a comment
There was a problem hiding this comment.
My reading of the diff is that there isn't integration test coverage for the functionality, and the tests in my opinion mock rather a lot of the relevant logic. I think the tests added here are good and valuable and should be retained but if the assessment is correct, adding an integration test would probably be useful as well (I have been using claude to create those lately).
BenchmarksBenchmark execution time: 2026-02-13 20:17:42 Comparing candidate commit 87ab619 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 43 metrics, 2 unstable metrics. scenario:profiling - gvl benchmark samples
|
| it 'includes process tags' do | ||
| Datadog.configure do |c| | ||
| c.service = 'test-service' | ||
| c.experimental_propagate_process_tags_enabled = true | ||
| end | ||
|
|
||
| expected_tags = Datadog::Core::Environment::Process.serialized | ||
|
|
||
| described_class.publish(Datadog.configuration) | ||
|
|
||
| expect(content).to include('process_tags' => expected_tags) | ||
| expect(expected_tags).not_to be_empty | ||
| end |
There was a problem hiding this comment.
It's a good practice, to leave in it only the part responsible for the assertion. And in RSpec we have tools to do so, here is a refactored way
before do
Datadog.configure do |c|
c.service = 'test-service'
c.experimental_propagate_process_tags_enabled = true
end
end
let(:expected_tags) { Datadog::Core::Environment::Process.serialized }
it 'includes process tags' do
expect { described_class.publish(Datadog.configuration) }.to
change { content }.to hash_including('process_tags' => expected_tags)
endalso there are some downsides of the existing checks
- Expected tags are unknown to the input and they asserted to be verified
- No transition check
4 participants
What does this PR do?
Follows the Java implementation of adding process tags and container id for AIDM-279.
The java code is here:
https://github.com/DataDog/dd-trace-java/blob/master/dd-trace-core/src/main/java/datadog/trace/core/servicediscovery/ServiceDiscovery.java#L37-L38
Where process tags are comma separated: "a, b"
Motivation:
AIDM-279
Change log entry
Yes. Add process tags and container id to process discovery payloads when the experimental setting DD_EXPERIMENTAL_PROPAGATE_PROCESS_TAGS_ENABLED=true is enabled.
Additional Notes:
How to test the change?
See AIDM-279 for screenshots of what it looks like on the Live Processes page.